From: Mike Hommey <mh@glandium.org>
Date: Sun, 24 Jun 2018 09:02:38 +0900
Subject: Bug 1470701 - Use run-time page size when changing mapping
 permissions in elfhack injected code. r?froydnj

When a binary has a PT_GNU_RELRO segment, the elfhack injected code
uses mprotect to add the writable flag to relocated pages before
applying relocations, removing it afterwards. To do so, the elfhack
program uses the location and size of the PT_GNU_RELRO segment, and
adjusts it to be aligned according to the PT_LOAD alignment.

The problem here is that the PT_LOAD alignment doesn't necessarily match
the actual page alignment, and the resulting mprotect may end up not
covering the full extent of what the dynamic linker has protected
read-only according to the PT_GNU_RELRO segment. In turn, this can lead
to a crash on startup when trying to apply relocations to the still
read-only locations.

Practically speaking, this doesn't end up being a problem on x86, where
the PT_LOAD alignment is usually 4096, which happens to be the page
size, but on Debian armhf, it is 64k, while the run time page size can be
4k.
---
 build/unix/elfhack/elfhack.cpp | 98 +++++++++++++++++++---------------
 build/unix/elfhack/inject.c    | 31 ++++++-----
 build/unix/elfhack/test.c      |  4 ++
 3 files changed, 78 insertions(+), 55 deletions(-)

diff --git a/build/unix/elfhack/elfhack.cpp b/build/unix/elfhack/elfhack.cpp
index 4d270af0e603..e7e2fa5b5ca4 100644
--- a/build/unix/elfhack/elfhack.cpp
+++ b/build/unix/elfhack/elfhack.cpp
@@ -87,12 +87,13 @@ class ElfRelHackCode_Section : public ElfSection {
  public:
   ElfRelHackCode_Section(Elf_Shdr &s, Elf &e,
                          ElfRelHack_Section &relhack_section, unsigned int init,
-                         unsigned int mprotect_cb)
+                         unsigned int mprotect_cb, unsigned int sysconf_cb)
       : ElfSection(s, nullptr, nullptr),
         parent(e),
         relhack_section(relhack_section),
         init(init),
-        mprotect_cb(mprotect_cb) {
+        mprotect_cb(mprotect_cb),
+        sysconf_cb(sysconf_cb) {
     std::string file(rundir);
     file += "/inject/";
     switch (parent.getMachine()) {
@@ -130,7 +131,6 @@ class ElfRelHackCode_Section : public ElfSection {
           "Couldn't find a symbol table for the injected code");
 
     relro = parent.getSegmentByType(PT_GNU_RELRO);
-    align = parent.getSegmentByType(PT_LOAD)->getAlign();
 
     // Find the init symbol
     entry_point = -1;
@@ -357,12 +357,12 @@ class ElfRelHackCode_Section : public ElfSection {
           addr = init;
         } else if (relro && strcmp(name, "mprotect_cb") == 0) {
           addr = mprotect_cb;
+        } else if (relro && strcmp(name, "sysconf_cb") == 0) {
+          addr = sysconf_cb;
         } else if (relro && strcmp(name, "relro_start") == 0) {
-          // Align relro segment start to the start of the page it starts in.
-          addr = relro->getAddr() & ~(align - 1);
-          // Align relro segment end to the start of the page it ends into.
+          addr = relro->getAddr();
         } else if (relro && strcmp(name, "relro_end") == 0) {
-          addr = (relro->getAddr() + relro->getMemSize()) & ~(align - 1);
+          addr = (relro->getAddr() + relro->getMemSize());
         } else if (strcmp(name, "_GLOBAL_OFFSET_TABLE_") == 0) {
           // We actually don't need a GOT, but need it as a reference for
           // GOTOFF relocations. We'll just use the start of the ELF file
@@ -418,9 +418,9 @@ class ElfRelHackCode_Section : public ElfSection {
   std::vector<ElfSection *> code;
   unsigned int init;
   unsigned int mprotect_cb;
+  unsigned int sysconf_cb;
   int entry_point;
   ElfSegment *relro;
-  unsigned int align;
 };
 
 unsigned int get_addend(Elf_Rel *rel, Elf *elf) {
@@ -727,6 +727,7 @@ int do_relocation_section(Elf *elf, unsigned int rel_type,
   }
 
   unsigned int mprotect_cb = 0;
+  unsigned int sysconf_cb = 0;
   // If there is a relro segment, our injected code will run after the linker
   // sets the corresponding pages read-only. We need to make our code change
   // that to read-write before applying relocations, which means it needs to
@@ -740,37 +741,48 @@ int do_relocation_section(Elf *elf, unsigned int rel_type,
   // section temporarily (it will be restored to a null value before any code
   // can actually use it)
   if (elf->getSegmentByType(PT_GNU_RELRO)) {
-    Elf_SymValue *mprotect = symtab->lookup("mprotect", STT(FUNC));
-    if (!mprotect) {
-      symtab->syms.emplace_back();
-      mprotect = &symtab->syms.back();
-      symtab->grow(symtab->syms.size() * symtab->getEntSize());
-      mprotect->name =
-          ((ElfStrtab_Section *)symtab->getLink())->getStr("mprotect");
-      mprotect->info = ELF32_ST_INFO(STB_GLOBAL, STT_FUNC);
-      mprotect->other = STV_DEFAULT;
-      new (&mprotect->value) ElfLocation(nullptr, 0, ElfLocation::ABSOLUTE);
-      mprotect->size = 0;
-      mprotect->defined = false;
-
-      // The DT_VERSYM data (in the .gnu.version section) has the same number of
-      // entries as the symbols table. Since we added one entry there, we need
-      // to add one entry here. Zeroes in the extra data means no version for
-      // that symbol, which is the simplest thing to do.
-      ElfSection *gnu_versym = dyn->getSectionForType(DT_VERSYM);
-      if (gnu_versym) {
-        gnu_versym->grow(gnu_versym->getSize() + gnu_versym->getEntSize());
+    ElfSection *gnu_versym = dyn->getSectionForType(DT_VERSYM);
+    auto lookup = [&symtab, &gnu_versym](const char *symbol) {
+      Elf_SymValue *sym_value = symtab->lookup(symbol, STT(FUNC));
+      if (!sym_value) {
+        symtab->syms.emplace_back();
+        sym_value = &symtab->syms.back();
+        symtab->grow(symtab->syms.size() * symtab->getEntSize());
+        sym_value->name =
+            ((ElfStrtab_Section *)symtab->getLink())->getStr(symbol);
+        sym_value->info = ELF32_ST_INFO(STB_GLOBAL, STT_FUNC);
+        sym_value->other = STV_DEFAULT;
+        new (&sym_value->value) ElfLocation(nullptr, 0, ElfLocation::ABSOLUTE);
+        sym_value->size = 0;
+        sym_value->defined = false;
+
+        // The DT_VERSYM data (in the .gnu.version section) has the same number
+        // of entries as the symbols table. Since we added one entry there, we
+        // need to add one entry here. Zeroes in the extra data means no version
+        // for that symbol, which is the simplest thing to do.
+        if (gnu_versym) {
+          gnu_versym->grow(gnu_versym->getSize() + gnu_versym->getEntSize());
+        }
       }
-    }
+      return sym_value;
+    };
 
-    // Add a relocation for the mprotect symbol.
-    new_rels.emplace_back();
-    Rel_Type &rel = new_rels.back();
-    memset(&rel, 0, sizeof(rel));
-    rel.r_info = ELF32_R_INFO(
-        std::distance(symtab->syms.begin(),
-                      std::vector<Elf_SymValue>::iterator(mprotect)),
-        rel_type2);
+    Elf_SymValue *mprotect = lookup("mprotect");
+    Elf_SymValue *sysconf = lookup("sysconf");
+
+    // Add relocations for the mprotect and sysconf symbols.
+    auto add_relocation_to = [&new_rels, &symtab, rel_type2](
+                                 Elf_SymValue *symbol, unsigned int location) {
+      new_rels.emplace_back();
+      Rel_Type &rel = new_rels.back();
+      memset(&rel, 0, sizeof(rel));
+      rel.r_info = ELF32_R_INFO(
+          std::distance(symtab->syms.begin(),
+                        std::vector<Elf_SymValue>::iterator(symbol)),
+          rel_type2);
+      rel.r_offset = location;
+      return location;
+    };
 
     // Find the beginning of the bss section, and use an aligned location in
     // there for the relocation.
@@ -782,13 +794,14 @@ int do_relocation_section(Elf *elf, unsigned int rel_type,
       size_t ptr_size = Elf_Addr::size(elf->getClass());
       size_t usable_start = (s->getAddr() + ptr_size - 1) & ~(ptr_size - 1);
       size_t usable_end = (s->getAddr() + s->getSize()) & ~(ptr_size - 1);
-      if (usable_end - usable_start >= ptr_size) {
-        mprotect_cb = rel.r_offset = usable_start;
+      if (usable_end - usable_start >= 2 * ptr_size) {
+        mprotect_cb = add_relocation_to(mprotect, usable_start);
+        sysconf_cb = add_relocation_to(sysconf, usable_start + ptr_size);
         break;
       }
     }
 
-    if (mprotect_cb == 0) {
+    if (mprotect_cb == 0 || sysconf_cb == 0) {
       fprintf(stderr, "Couldn't find .bss. Skipping\n");
       return -1;
     }
@@ -797,8 +810,9 @@ int do_relocation_section(Elf *elf, unsigned int rel_type,
   section->rels.assign(new_rels.begin(), new_rels.end());
   section->shrink(new_rels.size() * section->getEntSize());
 
-  ElfRelHackCode_Section *relhackcode = new ElfRelHackCode_Section(
-      relhackcode_section, *elf, *relhack, original_init, mprotect_cb);
+  ElfRelHackCode_Section *relhackcode =
+      new ElfRelHackCode_Section(relhackcode_section, *elf, *relhack,
+                                 original_init, mprotect_cb, sysconf_cb);
   // Find the first executable section, and insert the relhack code before
   // that. The relhack data is inserted between .rel.dyn and .rel.plt.
   ElfSection *first_executable = nullptr;
diff --git a/build/unix/elfhack/inject.c b/build/unix/elfhack/inject.c
index 862e9d9d9752..7a1f65fd4137 100644
--- a/build/unix/elfhack/inject.c
+++ b/build/unix/elfhack/inject.c
@@ -4,6 +4,7 @@
 
 #include <stdint.h>
 #include <stdlib.h>
+#include <unistd.h>
 #include <sys/mman.h>
 #include <elf.h>
 
@@ -29,6 +30,7 @@ extern __attribute__((visibility("hidden"))) Elf_Ehdr elf_header;
 extern __attribute__((visibility("hidden"))) int (*mprotect_cb)(void *addr,
                                                                 size_t len,
                                                                 int prot);
+extern __attribute__((visibility("hidden"))) long (*sysconf_cb)(int name);
 extern __attribute__((visibility("hidden"))) char relro_start[];
 extern __attribute__((visibility("hidden"))) char relro_end[];
 
@@ -58,34 +60,37 @@ __attribute__((section(".text._init"))) int init(int argc, char **argv,
   return 0;
 }
 
-static inline __attribute__((always_inline)) void relro_pre() {
+static inline __attribute__((always_inline)) void do_relocations_with_relro(
+    void) {
+  long page_size = sysconf_cb(_SC_PAGESIZE);
+  uintptr_t aligned_relro_start = ((uintptr_t)relro_start) & ~(page_size - 1);
+  uintptr_t aligned_relro_end = ((uintptr_t)relro_end) & ~(page_size - 1);
   // By the time the injected code runs, the relro segment is read-only. But
   // we want to apply relocations in it, so we set it r/w first. We'll restore
   // it to read-only in relro_post.
-  mprotect_cb(relro_start, relro_end - relro_start, PROT_READ | PROT_WRITE);
-}
+  mprotect_cb((void *)aligned_relro_start,
+              aligned_relro_end - aligned_relro_start, PROT_READ | PROT_WRITE);
+
+  do_relocations();
 
-static inline __attribute__((always_inline)) void relro_post() {
-  mprotect_cb(relro_start, relro_end - relro_start, PROT_READ);
-  // mprotect_cb is a pointer allocated in .bss, so we need to restore it to
-  // a NULL value.
+  mprotect_cb((void *)aligned_relro_start,
+              aligned_relro_end - aligned_relro_start, PROT_READ);
+  // mprotect_cb and sysconf_cb are allocated in .bss, so we need to restore
+  // them to a NULL value.
   mprotect_cb = NULL;
+  sysconf_cb = NULL;
 }
 
 __attribute__((section(".text._init_noinit_relro"))) int init_noinit_relro(
     int argc, char **argv, char **env) {
-  relro_pre();
-  do_relocations();
-  relro_post();
+  do_relocations_with_relro();
   return 0;
 }
 
 __attribute__((section(".text._init_relro"))) int init_relro(int argc,
                                                              char **argv,
                                                              char **env) {
-  relro_pre();
-  do_relocations();
-  relro_post();
+  do_relocations_with_relro();
   original_init(argc, argv, env);
   return 0;
 }
diff --git a/build/unix/elfhack/test.c b/build/unix/elfhack/test.c
index 3ca40b18e22e..ab994a45d2d2 100644
--- a/build/unix/elfhack/test.c
+++ b/build/unix/elfhack/test.c
@@ -127,6 +127,10 @@ int print_status() {
 __thread int foo;
 __thread long long int bar[512];
 
+/* We need a .bss that can hold at least 2 pointers. The static in
+ * end_test() plus this variable should do. */
+size_t dummy;
+
 void end_test() {
   static size_t count = 0;
   /* Only exit when both constructors have been called */
